SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after …#573
SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after …#573Alcamech wants to merge 1 commit intoopenshift:masterfrom
Conversation
…upgrade completes
openshift-ci-robot
added
the
jira/valid-reference
|
@Alcamech: This pull request references SREP-413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@Alcamech: This pull request references SREP-413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Alcamech The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #573 +/- ##
==========================================
+ Coverage 54.35% 54.47% +0.11%
==========================================
Files 123 123
Lines 6123 6139 +16
==========================================
+ Hits 3328 3344 +16
Misses 2592 2592
Partials 203 203
🚀 New features to boost your workflow:
|
|
@Alcamech: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@Alcamech: This pull request references SREP-413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| // the UpgradeConfig was recreated (e.g., by UpgradeConfigManager due to spec changes) | ||
| // after an upgrade finished, causing the status/history to be lost. | ||
| if cvClient.HasUpgradeCompleted(clusterVersion, instance) { | ||
| reqLogger.Info("Upgrade already completed for this version, setting phase to Upgraded") |
There was a problem hiding this comment.
With reference to https://github.com/openshift/managed-upgrade-operator/blob/master/pkg/upgraders/controlplanestep.go#L53-L64, CVO marking Upgrade completed was Control plane upgrade only but not worker plane upgrade. Thus that's being tracked separately. If indeed this has changed on CVO side as well then we would need to align the upgrade steps too which is a separate prerequisite change.
4 participants
SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after upgrade completes
What type of PR is this?
Bug
What this PR does / why we need it?
When an
UpgradeConfigis recreated by theUpgradeConfigManager(due to spec changes from OCM after an upgrade completes), the status/history is lost. The controller would then create a new history entry with Phase: New, run validation, and fail because the desired version matches the current version - triggering a falseUpgradeConfigValidationFailedSREalert.This PR adds a check for HasUpgradeCompleted() before defaulting to Phase: New. If CVO confirms the upgrade already completed, we create the history entry with Phase: Upgraded, bypassing validation entirely and preventing the false alert.
Which Jira/Github issue(s) this PR fixes?
SREP-413
Special notes for your reviewer:
If StartTime or CompleteTime cannot be extracted from CVO history (e.g., CVO's CompletionTime is nil), the synthetic history entry may have nil timestamps. This could cause a panic in reportUpgradeMetrics() at line 310. Should we add nil checks or default to time.Now() as a fallback ?
A/B Testing Results
Without the fix in place on a fresh CR containing the same version with no
.statussection I get the following logsWith the fix in place I get the following logs instead
Pending(stuck in loop)Upgraded(immediate)UpdateMetricValidationFailedfires